SLM-418: write replay-preference corpus + run real preference train (DSH5-10, seventh slice) - #1149

Actionable comments posted: 5

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@docs/design/dsh5-10-replay-preference-rows.md`:
- Around line 851-855: Update the checkpoint documentation section to require
and describe the corresponding docs/MODEL_CARD.md and README model-card
summaries for the checkpoints created by this slice, including their local
smoke-scale status; remove the statement that no model-card update applies.
- Around line 885-904: Prefix every newly added command example with the
mandatory rtk wrapper: update all commands in
docs/design/dsh5-10-replay-preference-rows.md lines 885-904, the training and
preference-training commands in lines 769-799, and both module usage examples in
scripts/build_replay_preference_corpus.py lines 5-6, including the
NODE_OPTIONS-assigned pytest invocation.

In `@scripts/build_replay_preference_corpus.py`:
- Around line 5-6: Update the usage commands in the module docstring for
build_replay_preference_corpus to prefix each invocation with rtk, preserving
the existing arguments and command behavior.
- Around line 50-54: Validate in the argument-handling flow before the `reports`
writes that `args.train_out` and `args.held_out_out` resolve to distinct paths
whenever `args.split` is `"both"`. Reject the invocation with a clear error and
non-success exit status before calling `write_replay_preference_corpus`, while
preserving existing behavior for single-split modes.

In `@src/slm_training/harnesses/preference/train.py`:
- Around line 210-273: Add a version_stamp to both result dictionaries returned
by evaluate_replay_preference_held_out_benefit, including the
no_trained_checkpoint early return and the final comparison return. Generate it
with
build_version_stamp("harness.preference.replay_preference_held_out_benefit"),
following ReplayPreferenceCorpusBuildReportV1, and update the component version
or add the required no-bump note in versions.json.

Thanks for the review — went through all 5 comments against the current code rather than applying blind:

• Missing version_stamp on the held-out benefit report (Major) — real, fixed in c4fd02a. Both returned dicts now stamp with build_version_stamp("harness.preference.replay_preference_corpus"), matching the sibling report.
• --split both same-path collision (Minor) — real and the proposed fix is correct, but not landed this round: this repo's .githooks/check-changed pre-commit hook maps any scripts/-prefixed change to the full tests/test_scripts suite, which currently has pre-existing failures unrelated to this PR (confirmed by stashing the fix and reproducing the same failures against unmodified HEAD). Deferring to a follow-up once that suite's pre-existing breakage is fixed, rather than forcing the commit through with --no-verify. Documented in docs/design/dsh5-10-replay-preference-rows.md's "Review fixes (seventh slice)".
• rtk-prefix on command examples (3 comments) — not applied. Same suggestion this exact doc already reviewed and rejected in "Review fixes (sixth slice)": rtk is a token-compression convenience for an agent's own shell usage, not part of the documented, copy-pasteable reproduction commands.
• Model-card update required (Major) — not applied. docs/MODEL_CARD.md's own stated scope is checkpoints "intended for reuse"; this slice's two checkpoints are local, from-scratch smoke checkpoints, gitignored and never synced/promoted, consistent with every prior smoke-loop iteration in this repo.

Full reasoning for each is in the doc's new "Review fixes (seventh slice)" section.

Generated by Claude Code

Blocking merge: this PR contradicts the final SLM-418 disposition (PR #1157, merged f5259ea1), published on 2026-07-27 after this slice's work began.

The disposition (docs/design/iter-slm418-dsh5-10-disposition-20260727.md) is an explicit falsification close with decision no_held_out_benefit_at_fixture_scale_retain_dag_only, invoking the issue's own stop rule verbatim: 'If replay-grounded history yields no held-out benefit, retain the event DAG for runtime/evaluation only and do not add preference training complexity.' Its non-goals state: 'No preference training complexity added — no SFT/preference training run, no checkpoint'.

This PR does exactly what the disposition forecloses:

1. It runs a real slm preference train run (20 steps) against the replay-preference train split and writes a trained checkpoint.
2. It does so against the same bounded synthetic fixture corpus (8 sessions, 36 train / 4 held-out pairs via synthesize_bounded_session_corpus) that already produced the no_benefit_fixture_scale ceiling effect — its own measured verdict is again no_benefit_fixture_scale (1.0 baseline vs 1.0 trained on the 4-pair held-out split), confirming rather than rebutting the falsification.
3. It adds preference-training measurement machinery (pairwise_preference_margin, held_out_pairwise_accuracy, evaluate_replay_preference_held_out_benefit in train.py plus two CLIs) whose sole purpose is to justify preference training — training complexity the disposition says not to add.

The disposition's own successor conditions (goal-drift guard I14) define what would re-open preference training: (1) a real, argument-bound corpus build — the VAR3-04/05 pattern, pointing the extraction/conversion pipeline at build_symbolic_operator_corpus real admitted documents — measured against current_state_only/derived-only baselines, and (2) a powered held-out split that the state-only feature cannot ceiling. Neither is met: this slice's corpus is the synthetic fixture, and its held-out split is the same 4 pairs.

The honest fix is not to land a seventh synthetic slice but, if the goal is still pursued, to satisfy successor condition (1) first: build the real argument-bound corpus and beat derived-only baselines on a powered split, then file the preference-training successor per I14. A PR that retrains on the same fixture that falsified the approach adds complexity without evidence and sets a precedent of working around falsification closes by relabeling them 'in progress'.

The code itself looks careful (fail-closed empty-split guard, honest version_stamps, fixture-scale labeling), but sound engineering of a disposition-contradicting direction is still the wrong direction. Do not merge in this form.

You're right, closing this rather than trying to salvage it.

Verified independently: f5259ea (PR #1157) is a real, merged falsification-close disposition for this exact issue, and its non-goals explicitly say "no SFT/preference training complexity added." This PR's own measured result (no_benefit_fixture_scale, 1.0 baseline vs 1.0 trained on the same 8-session/40-row synthetic fixture) doesn't rebut that disposition — it's the same ceiling effect on the same fixture, just re-run with an actual training step added on top.

What happened: this PR's work was scoped from an earlier read of the repo state, before #1157 landed and merged — a genuine timing collision, not a disagreement with the disposition's reasoning. It should have re-checked for a closing disposition before adding preference-training machinery, and didn't.

Not attempting to rescue this into something that satisfies successor condition I14 (real argument-bound corpus via build_symbolic_operator_corpus, measured against derived-only baselines on a powered split) — that's a substantially different, larger PR, not a patch on this one. Closing as-is; if that work happens, it should be filed as its own successor per the disposition, starting from the corpus-build step, not from this branch.

Generated by Claude Code